Skip to content

[dotnet] [bidi] Align Scipt.LocalValue.Map with spec, enable negative zero #15395

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 30 commits into from
Mar 15, 2025
Merged

[dotnet] [bidi] Align Scipt.LocalValue.Map with spec, enable negative zero #15395

merged 30 commits into from
Mar 15, 2025
+584 −8

Conversation

@RenderMichael
Copy link
Contributor

@RenderMichael RenderMichael commented Mar 9, 2025 •
edited
Loading

User description

Fixes using the Script.LocalValue.Map type and the related return type. The spec describes it the same as Object:
image

So this is now a "clone" of LocalValue.Object.
 
Additionally, this PR deserializes negative zero from a string, in alignment with the spec:
image

Motivation and Context

Fixes #15394

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Checklist

  • I have read the contributing document.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I have added tests to cover my changes.
  • All new and existing tests passed.

PR Type

Bug fix, Tests

Description

  • Fixed Script.LocalValue.Map to align with the WebDriver BiDi spec.

  • Updated RemoteValue.Map to use IReadOnlyList<IReadOnlyList<RemoteValue>>.

  • Added comprehensive tests for LocalValue and RemoteValue roundtrip scenarios.

  • Enhanced JSON serialization options to handle named floating-point literals.

Changes walkthrough 📝

Relevant files
Enhancement
Broker.cs
Enhanced JSON serialization options.                                         

dotnet/src/webdriver/BiDi/Communication/Broker.cs

  • Added support for named floating-point literals in JSON serialization.
    		• +1/-0     
    Bug fix
    LocalValue.cs
    Updated `LocalValue.Map` structure.                                           

    dotnet/src/webdriver/BiDi/Modules/Script/LocalValue.cs

  • Changed Map to use IEnumerable>.
  • Ensured alignment with Object serialization structure.
    		• +1/-1     
    RemoteValue.cs
    Modified `RemoteValue.Map` structure.                                       

    dotnet/src/webdriver/BiDi/Modules/Script/RemoteValue.cs

  • Updated Map to use IReadOnlyList>.
  • Ensured compatibility with the WebDriver BiDi spec.
    		• +1/-1     
    Tests
    CallFunctionParameterTest.cs
    Added comprehensive tests for `LocalValue` and `RemoteValue`.

    dotnet/test/common/BiDi/Script/CallFunctionParameterTest.cs

  • Added new test cases for LocalValue and RemoteValue roundtrip
    scenarios.
  • Verified serialization and deserialization of various LocalValue
    types.
  • Tested Map and Object structures for compliance with the spec.
    		• +139/-0 
    Need help?
  • Type /help how to ... in the comments thread for any questions about Qodo Merge usage.
  • Check out the documentation for more information.
    •

    @qodo-merge-pro
    Copy link
    Contributor

    qodo-merge-pro bot commented Mar 9, 2025

    PR Reviewer Guide 🔍

    Here are some key observations to aid the review process:

    🎫 Ticket compliance analysis ✅

    15394 - Fully compliant

    Compliant requirements:

    • Fix BiDi Script.LocalValue.Map to be passable to the Script.CallFunctionAsync command
    • Align Map serialization with the WebDriver BiDi spec, which states it should be serialized the same way as Object
    • Fix related RemoteValue.Map type to match the spec

    ⏱️ Estimated effort to review: 2 🔵🔵⚪⚪⚪
    🧪 PR contains tests
    🔒 No security concerns identified
    ⚡ Recommended focus areas for review

    Commented Test Cases

    There are two commented-out test cases for Boolean values (true/false) in the RoundtripOptions method. These should either be uncommented or removed if they're not needed.

     
    //yield return new TestCaseData(new LocalValue.Boolean(true), new RemoteValue.Boolean(true), "typeof arg === true")
//{
//    TestName = nameof(CanCallFunctionAndRoundtrip_Five) + "(true)",
//};
//yield return new TestCaseData(new LocalValue.Boolean(false), new RemoteValue.Boolean(false), "typeof arg === false")
//{
//    TestName = nameof(CanCallFunctionAndRoundtrip_Five) + "(false)",
//};

    @qodo-merge-pro
    Copy link
    Contributor

    qodo-merge-pro bot commented Mar 9, 2025 •
    edited
    Loading

    PR Code Suggestions ✨

    Explore these optional code suggestions:

    CategorySuggestion                                                                                                                                    Impact
    Possible issue
    ✅ Fix boolean assertion logic
    Suggestion Impact:The suggestion pointed out a logical error in the JavaScript assertion for boolean values. The commit completely restructured the test approach, removing the problematic code that used the incorrect assertion pattern. The commented-out test cases with the incorrect assertions were removed entirely in the commit.

    code diff:

    -        //yield return new TestCaseData(new LocalValue.Boolean(true), new RemoteValue.Boolean(true), "typeof arg === true")
-        //{
-        //    TestName = nameof(CanCallFunctionAndRoundtrip_Five) + "(true)",
-        //};
-        //yield return new TestCaseData(new LocalValue.Boolean(false), new RemoteValue.Boolean(false), "typeof arg === false")
-        //{
-        //    TestName = nameof(CanCallFunctionAndRoundtrip_Five) + "(false)",
-        //};

    The JavaScript assertion for Boolean values is incorrect. The typeof operator
    returns the string "boolean" for boolean values, and you're comparing it
    directly to true/false. You should either remove the typeof or change the
    assertion to check the value directly.

    dotnet/test/common/BiDi/Script/CallFunctionParameterTest.cs [275-282]

    -//yield return new TestCaseData(new LocalValue.Boolean(true), new RemoteValue.Boolean(true), "typeof arg === true")
+//yield return new TestCaseData(new LocalValue.Boolean(true), new RemoteValue.Boolean(true), "arg === true")
 //{
 //    TestName = nameof(CanCallFunctionAndRoundtrip_Five) + "(true)",
 //};
-//yield return new TestCaseData(new LocalValue.Boolean(false), new RemoteValue.Boolean(false), "typeof arg === false")
+//yield return new TestCaseData(new LocalValue.Boolean(false), new RemoteValue.Boolean(false), "arg === false")
 //{
 //    TestName = nameof(CanCallFunctionAndRoundtrip_Five) + "(false)",
 //};

    [Suggestion has been applied]

    Suggestion importance[1-10]: 8

    __

    Why: The suggestion correctly identifies a logical error in the commented-out test cases. The JavaScript assertion "typeof arg === true" is incorrect as typeof would return the string "boolean", not the boolean value itself. This would cause the tests to fail if uncommented.

    		Medium
    • Update

    RenderMichael
    RenderMichael commented Mar 9, 2025
    View reviewed changes
    RenderMichael
    RenderMichael commented Mar 9, 2025
    View reviewed changes
    RenderMichael
    RenderMichael commented Mar 9, 2025
    View reviewed changes
    @RenderMichael RenderMichael changed the title [dotnet] Align Scipt.LocalValue.Map with spec [dotnet] [bidi] Align Scipt.LocalValue.Map with spec Mar 9, 2025
    RenderMichael
    RenderMichael commented Mar 9, 2025
    View reviewed changes
    RenderMichael
    RenderMichael commented Mar 9, 2025
    View reviewed changes
    nvborisenko
    nvborisenko reviewed Mar 9, 2025
    View reviewed changes
    nvborisenko
    nvborisenko reviewed Mar 9, 2025
    View reviewed changes
    RenderMichael
    RenderMichael commented Mar 10, 2025
    View reviewed changes
    RenderMichael
    RenderMichael commented Mar 10, 2025
    View reviewed changes
    nvborisenko
    nvborisenko reviewed Mar 10, 2025
    View reviewed changes
    nvborisenko
    nvborisenko reviewed Mar 10, 2025
    View reviewed changes
    @nvborisenko
    Copy link
    Member

    nvborisenko commented Mar 12, 2025

    When arguments are serialized as "arguments":[{"type":"number","value":-0}] On Firefox: Error: Assert failed: 0 (in other words, it thinks the value is regular 0)

    On my Firefox your test passes when we {"type":"number","value":-0}

    @nvborisenko
    Copy link
    Member

    nvborisenko commented Mar 12, 2025

    OK, so in Firefox everything works good without any custom double converters. Chromium cannot handle -0 and everything works good.

    I posted w3c/webdriver-bidi#887 issue in BiDi spec. I propose to not introduce custom converter at this time, and move forward with ignored unit tests (for chromium only).

    PS: I was wrong, spec requires -0 to be string:

    [Type](https://tc39.es/ecma262/#sec-ecmascript-data-types-and-values)(value) is Number
Switch on the value of value:

NaN
Let serialized be "NaN"
-0
Let serialized be "-0"

    nvborisenko
    nvborisenko reviewed Mar 12, 2025
    View reviewed changes
    nvborisenko
    nvborisenko reviewed Mar 12, 2025
    View reviewed changes
    nvborisenko
    nvborisenko reviewed Mar 12, 2025
    View reviewed changes
    nvborisenko
    nvborisenko reviewed Mar 12, 2025
    View reviewed changes
    nvborisenko
    nvborisenko reviewed Mar 12, 2025
    View reviewed changes
    nvborisenko
    nvborisenko reviewed Mar 12, 2025
    View reviewed changes
    nvborisenko
    nvborisenko reviewed Mar 12, 2025
    View reviewed changes
    @RenderMichael
    Copy link
    Contributor Author

    RenderMichael commented Mar 13, 2025

    Formatting build failure unrelated:

    --- a/py/test/selenium/webdriver/common/selenium_manager_tests.py
+++ b/py/test/selenium/webdriver/common/selenium_manager_tests.py

    Test failures not related:

    //py:common-chrome-bidi-test/selenium/webdriver/common/bidi_tests.py
//py:common-edge-bidi-test/selenium/webdriver/common/bidi_tests.py
//rb/spec/integration/selenium/webdriver:action_builder-chrome-remote
//rb/spec/integration/selenium/webdriver:driver-chrome
//rb/spec/integration/selenium/webdriver:driver-chrome-remote
//rb/spec/integration/selenium/webdriver:driver-firefox-beta
//rb/spec/integration/selenium/webdriver:manager-chrome
//rb/spec/integration/selenium/webdriver:select-chrome
//rb/spec/integration/selenium/webdriver:select-chrome-remote

    nvborisenko
    nvborisenko approved these changes Mar 15, 2025
    View reviewed changes
    Copy link
    Member

    @nvborisenko nvborisenko left a comment

    Choose a reason for hiding this comment

    The reason will be displayed to describe this comment to others. Learn more.

    Good test coverage, thanks!

    @nvborisenko nvborisenko merged commit 2e0f84e into SeleniumHQ:trunk Mar 15, 2025
    8 of 10 checks passed
    @RenderMichael RenderMichael deleted the bidi-map branch March 15, 2025 12:58
    sandeepsuryaprasad pushed a commit to sandeepsuryaprasad/selenium that referenced this pull request Mar 23, 2025
    @RenderMichael @sandeepsuryaprasad
    [dotnet] [bidi] Align Scipt.LocalValue.Map with spec (SeleniumHQ#15395
    226d4b8 
    )
    @dependabot dependabot bot mentioned this pull request Sep 18, 2025
    Merged
    @dependabot dependabot bot mentioned this pull request Oct 1, 2025
    Closed
    @dependabot dependabot bot mentioned this pull request Oct 24, 2025
    Closed
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

    Reviewers

    @nvborisenko nvborisenko nvborisenko approved these changes

    Assignees

    No one assigned

    Labels

    Review effort 2/5

    Projects

    None yet

    Milestone

    No milestone

    Development

    Successfully merging this pull request may close these issues.

    [🐛 Bug]: [dotnet] BiDi LocalValue.Map cannot be passed to the Script.CallFunctionAsync command

    2 participants

    @RenderMichael @nvborisenko